Fix counting of escape sequences when splitting TXT strings #1540

janik-cloudflare · 2024-03-01T12:57:59Z

endingToTxtSlice, used by TXT, SPF and a few other types, parses a string such as "hello world" from an RR's content in a zone file. These strings are limited to 255 characters, and endingToTxtSlice automatically splits them if they're longer than that. However, it didn't count the length correctly: escape sequences such as \\ or \123 were counted as multiple characters (2 and 4 respectively in these examples), but they should only count as one character because they represent a single byte in wire format (which is where this 255 character limit comes from). This commit fixes that.

janik-cloudflare · 2024-03-01T13:06:49Z

A more detailed explanation of the problem: #1384 (comment)

janik-cloudflare · 2024-03-22T09:13:18Z

Apologies, found a bug! Fix and more tests coming today.

Edit: PR updated.

`endingToTxtSlice`, used by TXT, SPF and a few other types, parses a string such as `"hello world"` from an RR's content in a zone file. These strings are limited to 255 characters, and `endingToTxtSlice` automatically splits them if they're longer than that. However, it didn't count the length correctly: escape sequences such as `\\` or `\123` were counted as multiple characters (2 and 4 respectively in these examples), but they should only count as one character because they represent a single byte in wire format (which is where this 255 character limit comes from). This commit fixes that.

miekg · 2024-03-29T18:01:22Z

I think this is OK (my gawd... txt records)... I see a escapedStringOffset without looking too closely, don't we already have a bunch of those type of functions? Is a new one really needed or can something older be re-used?

janik-cloudflare · 2024-03-30T18:18:05Z

Thanks for looking! I had indeed found one, escapedNameLen, but, while it's similar, it's also different enough to make it difficult to reuse for our purposes. Looking at callers of isDDD again now, I also don't see anything else that could easily be reused.

janik-cloudflare · 2024-04-15T11:02:01Z

FYI: we've been running this in production for 3+ weeks with no issues

catenacyber · 2024-04-19T06:34:51Z

Seems to have triggered https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68124 cc @miekg

miekg · 2024-04-19T06:48:42Z

hmm, thanks.

After some clicking this is the testcase:

 hinFo ÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿ\      
 ~~~

janik-cloudflare · 2024-04-19T09:49:11Z

Yikes, thanks @catenacyber. I don't have access to the report but I'll try to figure out what's happening here.

janik-cloudflare · 2024-04-19T10:31:52Z

@miekg I'm using slightly modified versions of Fuzz and FuzzNewRR (surely the latter is the problematic one), but I'm not able to replicate the issue. I've tried some variations of the test case, such as removing the spaces at the start of each of the two lines, removing the six spaces at the end of the first line, in case those were added by mistake, but no luck. I also tried the second line with and without a trailing \n. Parsing fails if I remove the initial spaces, but I imagine the report isn't about a simple error (fuzz function returning code 0) but about a panic.

I've also looked over HINFO's parse() in case this is a type-specific issue, but it doesn't look like it.

I think I need some help here. If the test case is available as a binary file that'd be great. Confirmation that it is indeed FuzzNewRR that's failing would be helpful too. Also, is there a stack trace or panic message available?

catenacyber · 2024-04-19T11:33:13Z

Here is the base64-encoded file

IGhpbkZvIP//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////XA==

FuzzNewRR is the target


panic: runtime error: slice bounds out of range [:256] with length 255
--
  |  
  | goroutine 17 [running, locked to thread]:
  | github.com/miekg/dns.endingToTxtSlice(0x0?, {0x755ec9, 0x10})
  | github.com/miekg/dns/scan_rr.go:60 +0xc78
  | github.com/miekg/dns.(*HINFO).parse(0x10c0000dc000, 0x10c00007c6c0?, {0x0?, 0x0?})
  | github.com/miekg/dns/scan_rr.go:201 +0x4b
  | github.com/miekg/dns.(*ZoneParser).Next(0x10c0000d2090)
  | github.com/miekg/dns/scan.go:672 +0x29b7
  | github.com/miekg/dns.ReadRR({0x810638?, 0x10c0000ce000?}, {0x0?, 0x797180?})
  | github.com/miekg/dns/scan.go:128 +0xa6
  | github.com/miekg/dns.NewRR({0x10c0000c8000, 0x106})
  | github.com/miekg/dns/scan.go:113 +0x13f
  | github.com/miekg/dns.FuzzNewRR({0x612000000940?, 0x0?, 0x5cc301?})
  | github.com/miekg/dns/fuzz.go:29 +0xdb
  | main.LLVMFuzzerTestOneInput(...)
  | ./main.1703470873.go:21

janik-cloudflare · 2024-04-19T12:24:10Z

Thanks a ton! The problem seems to be within escapedStringOffset, in particular the return i + 1 within if offset >= byteOffset. The function is getting confused by the trailing slash and returning an OOB offset, which should never be allowed to happen.

It should be easy enough to fix by adding a bounds check there but I'll see if I can come up with something better that's easier to follow.

Smaller test case: escapedStringOffset("aa\\", 3) returns 4.

janik-cloudflare · 2024-04-19T12:37:39Z

Haven't tested it, but I think escapedNameLen might suffer from a similar issue where an invalid message could be produced if the last character is a backslash. That one is probably less serious because I doubt it would panic and, regardless, a backslash shouldn't be able to get there in the first place unless someone is modifying the structs directly. Perhaps another argument for not trying to preserve zone file syntax such as escape sequences in parsed data structures though?

janik-cloudflare · 2024-04-19T13:09:57Z

Here's my proposed fix: #1557

miekg · 2024-04-25T14:03:33Z

Looks ok. Not near a computer for a couple of days.. Are you willing to accept an invite to become maintainer and able to merge? /Miek

…

On Fri, 19 Apr 2024, 15:10 Janik Rabe, ***@***.***> wrote: Here's my proposed fix: #1557 <#1557> — Reply to this email directly, view it on GitHub <#1540 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWIW3LQBKBHHSO4XBE2MLY6EJTZAVCNFSM6AAAAABEBXPUECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRWGU2DMMJQGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

janik-cloudflare · 2024-04-26T08:23:49Z

Thank you Miek! It would be an honor!

miekg · 2024-04-26T20:13:27Z

Ack. Ack On mobile, but will push buttons next week. Thanks all, appreciate all the help

…

On Fri, 26 Apr 2024, 09:24 Janik Rabe, ***@***.***> wrote: Thank you Miek! It would be an honor! — Reply to this email directly, view it on GitHub <#1540 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWIW34NQOYKLH2CHF6J33Y7IFKXAVCNFSM6AAAAABEBXPUECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZYHA4DSNZRGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

miekg · 2024-06-13T14:14:27Z

invite finally sent

* Swap closing order in `inAxfr` and `inIxfr` (miekg#1511) * Fix closing order * Comment to make clear that the close order is deliberate --------- Co-authored-by: Tim Scheuermann <[email protected]> * feat: add support for ReuseAddr (miekg#1510) * feat: add support for ReuseAddr * Update listen_reuseport.go * Update listen_reuseport.go * fixup! feat: add support for ReuseAddr --------- Co-authored-by: Miek Gieben <[email protected]> * Release 1.1.57 * Try explaining duplicate RCODEs Add extra link to the docs for the duplicate Rcode entries See miekg#1523 Signed-off-by: Miek Gieben <[email protected]> * docs: added ninedos to readme (miekg#1522) * Allow use of fs.FS for $INCLUDE and wrap errors (miekg#1526) * Allow use of fs.FS for $INCLUDE and wrap errors This adds ZoneParser.SetIncludeAllowedFS, to specify an fs.FS when enabling support for $INCLUDE, for reading included files from somewhere other than the local filesystem. I've also modified ParseError to support wrapping another error, such as errors encountered while opening the $INCLUDE target. This allows for much more robust handling, using errors.Is() instead of testing for particular strings (which may not be identical between fs.FS implementations). ParseError was being constructed in a lot of places using positional instead of named members. Updating ParseError initialization after the new member field was added makes this change seem a lot larger than it actually is. The changes here should be completely backwards compatible. The ParseError change should be invisible to anyone not trying to unwrap it, and ZoneParser will continue to use os.Open if the existing SetIncludeAllowed method is called instead of the new SetIncludeAllowedFS method. * Don't duplicate SetIncludeAllowed; clarify edge cases Rather than duplicate functionality between SetIncludeAllowed and SetIncludeAllowedFS, have a method SetIncludeFS, which only sets the fs.FS. I've improved the documentation to point out some considerations for users hoping to use fs.FS as a security boundary. Per the fs.ValidPath documentation, fs.FS implementations must use path (not filepath) semantics, with slash as a separator (even on Windows). Some, like os.DirFS, also require all paths to be relative. I've clarified this in the documentation, made the includePath manipulation more robust to edge cases, and added some additional tests for relative and absolute paths. * Bump golang.org/x/net from 0.17.0 to 0.19.0 (miekg#1520) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.17.0 to 0.19.0. - [Commits](golang/net@v0.17.0...v0.19.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/sys from 0.13.0 to 0.15.0 (miekg#1518) Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.13.0 to 0.15.0. - [Commits](golang/sys@v0.13.0...v0.15.0) --- updated-dependencies: - dependency-name: golang.org/x/sys dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Add NXT record (miekg#1516) This add the NXT record (2535) to implement all records from the RFC. Also does a s/RFC RFC/RFC/ as I happen to bumb into that will editing the comments. Signed-off-by: Miek Gieben <[email protected]> * Add ISDN record (miekg#1515) We had the type code, this add the rest. Other RRs from 1183 are also fully impl. don't know why this one wasn't. Signed-off-by: Miek Gieben <[email protected]> * Bump golang.org/x/tools from 0.13.0 to 0.17.0 (miekg#1529) Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.13.0 to 0.17.0. - [Release notes](https://github.com/golang/tools/releases) - [Commits](golang/tools@v0.13.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/tools dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Release 1.1.58 * Improve NewRR documentation (miekg#1531) In particular, document the default origin. * Add incus to the list of users (miekg#1535) * Add option to do a zone transfer via TLS (miekg#1533) * New func InTLS Perform zone transfer via TLS * Test xfr via TLS * New field TLS, used to transfer via TLS --------- Co-authored-by: Cesar Kuroiwa <[email protected]> * IsDomainName: check for escape as last character (miekg#1532) Keep track if the escape, if still true when returning isDomainName should return false. TODO: - Should still be done in packDomainName as well. - And that should be tested - Some tests now fail There are multiple other places that supposedly also check for this, but they are not called in the parsing. Fixes: miekg#1528 Signed-off-by: Miek Gieben <[email protected]> * Bump golang.org/x/sys from 0.16.0 to 0.17.0 (miekg#1541) Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.16.0 to 0.17.0. - [Commits](golang/sys@v0.16.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/sys dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/net from 0.20.0 to 0.21.0 (miekg#1542) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.20.0 to 0.21.0. - [Commits](golang/net@v0.20.0...v0.21.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/tools from 0.17.0 to 0.19.0 (miekg#1551) Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.17.0 to 0.19.0. - [Release notes](https://github.com/golang/tools/releases) - [Commits](golang/tools@v0.17.0...v0.19.0) --- updated-dependencies: - dependency-name: golang.org/x/tools dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore: fix some comments (miekg#1547) Signed-off-by: xiaoxiangxianzi <[email protected]> * Add ifconfig.es to the list of users (miekg#1554) * Fix counting of escape sequences when splitting TXT strings (miekg#1540) `endingToTxtSlice`, used by TXT, SPF and a few other types, parses a string such as `"hello world"` from an RR's content in a zone file. These strings are limited to 255 characters, and `endingToTxtSlice` automatically splits them if they're longer than that. However, it didn't count the length correctly: escape sequences such as `\\` or `\123` were counted as multiple characters (2 and 4 respectively in these examples), but they should only count as one character because they represent a single byte in wire format (which is where this 255 character limit comes from). This commit fixes that. * Fix possible out-of-bounds read in endingToTxtSlice (miekg#1557) * Update escapedStringOffset to improve readability This function was, admittedly, a little difficult to follow. This new version is slightly more verbose, but, in my opinion, easier to understand. * Fix possible out-of-bounds read in endingToTxtSlice caused by escapedStringOffset If the input had a trailing backslash (normally the start of an escape sequence) with nothing following it, `escapedStringOffset` would return the length of the input, plus one (!), as the result index, causing an out-of-bounds read and panic in `endingToTxtSlice`. Consistent with, e.g., commit 2230854, I've decided to make this an error since it definitely indicates that the string isn't valid. Credit to OSS-Fuzz -- thank you! * Bump golang.org/x/sys from 0.18.0 to 0.20.0 (miekg#1571) Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.18.0 to 0.20.0. - [Commits](golang/sys@v0.18.0...v0.20.0) --- updated-dependencies: - dependency-name: golang.org/x/sys dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/net from 0.22.0 to 0.25.0 (miekg#1569) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.22.0 to 0.25.0. - [Commits](golang/net@v0.22.0...v0.25.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/tools from 0.19.0 to 0.22.0 (miekg#1574) Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.19.0 to 0.22.0. - [Release notes](https://github.com/golang/tools/releases) - [Commits](golang/tools@v0.19.0...v0.22.0) --- updated-dependencies: - dependency-name: golang.org/x/tools dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * (*Transfer) Out: Increment WaitGroup in example (miekg#1572) * Add a hook to catch invalid messages (miekg#1568) * Add a hook to catch invalid messages Currently there are hooks for reading messages off the wire (DecorateReader), checking if they comply with policy (MsgAcceptFunc), and generating responses (Handler). However, there is no hook that notifies the server when a message is dropped or rejected due to a syntax error. That makes it hard to monitor these packets without repeating the parsing process. This PR adds a hook for notifications about invalid packets. * s/InvalidMsg/MsgInvalid/g * These two too Signed-off-by: Miek Gieben <[email protected]> * Add RFC 9540 oblivious services via service binding records (miekg#1567) * update list of RFCs Signed-off-by: Miek Gieben <[email protected]> * add rfc3596 to the list (miekg#1577) --------- Signed-off-by: Miek Gieben <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: xiaoxiangxianzi <[email protected]> Co-authored-by: Tim Scheuermann <[email protected]> Co-authored-by: Tim Scheuermann <[email protected]> Co-authored-by: Jim <[email protected]> Co-authored-by: Miek Gieben <[email protected]> Co-authored-by: WintBit <[email protected]> Co-authored-by: Dave Pifke <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Richard Gibson <[email protected]> Co-authored-by: montag451 <[email protected]> Co-authored-by: Cesar Kuroiwa <[email protected]> Co-authored-by: Cesar Kuroiwa <[email protected]> Co-authored-by: xiaoxiangxianzi <[email protected]> Co-authored-by: dcarrillo <[email protected]> Co-authored-by: Janik Rabe <[email protected]> Co-authored-by: Patrik Lundin <[email protected]> Co-authored-by: Benjamin M. Schwartz <[email protected]> Co-authored-by: Steffen Sassalla <[email protected]> Co-authored-by: Infinoid <[email protected]>

janik-cloudflare requested review from miekg and tmthrgd as code owners March 1, 2024 12:58

janik-cloudflare mentioned this pull request Mar 1, 2024

Parsing TXT records does not properly de-escape quotes #1384

Open

janik-cloudflare force-pushed the txt-counting branch from d3869e0 to 00d0072 Compare March 22, 2024 09:58

miekg merged commit e4ef594 into miekg:master Apr 17, 2024
4 checks passed

janik-cloudflare mentioned this pull request Apr 19, 2024

Fix possible out-of-bounds read in endingToTxtSlice #1557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix counting of escape sequences when splitting TXT strings #1540

Fix counting of escape sequences when splitting TXT strings #1540

janik-cloudflare commented Mar 1, 2024

janik-cloudflare commented Mar 1, 2024

janik-cloudflare commented Mar 22, 2024 •

edited

Loading

miekg commented Mar 29, 2024

janik-cloudflare commented Mar 30, 2024

janik-cloudflare commented Apr 15, 2024

catenacyber commented Apr 19, 2024

miekg commented Apr 19, 2024

janik-cloudflare commented Apr 19, 2024

janik-cloudflare commented Apr 19, 2024 •

edited

Loading

catenacyber commented Apr 19, 2024

janik-cloudflare commented Apr 19, 2024 •

edited

Loading

janik-cloudflare commented Apr 19, 2024

janik-cloudflare commented Apr 19, 2024

miekg commented Apr 25, 2024 via email

janik-cloudflare commented Apr 26, 2024

miekg commented Apr 26, 2024 via email

miekg commented Jun 13, 2024

Fix counting of escape sequences when splitting TXT strings #1540

Fix counting of escape sequences when splitting TXT strings #1540

Conversation

janik-cloudflare commented Mar 1, 2024

janik-cloudflare commented Mar 1, 2024

janik-cloudflare commented Mar 22, 2024 • edited Loading

miekg commented Mar 29, 2024

janik-cloudflare commented Mar 30, 2024

janik-cloudflare commented Apr 15, 2024

catenacyber commented Apr 19, 2024

miekg commented Apr 19, 2024

janik-cloudflare commented Apr 19, 2024

janik-cloudflare commented Apr 19, 2024 • edited Loading

catenacyber commented Apr 19, 2024

janik-cloudflare commented Apr 19, 2024 • edited Loading

janik-cloudflare commented Apr 19, 2024

janik-cloudflare commented Apr 19, 2024

miekg commented Apr 25, 2024 via email

janik-cloudflare commented Apr 26, 2024

miekg commented Apr 26, 2024 via email

miekg commented Jun 13, 2024

janik-cloudflare commented Mar 22, 2024 •

edited

Loading

janik-cloudflare commented Apr 19, 2024 •

edited

Loading

janik-cloudflare commented Apr 19, 2024 •

edited

Loading